test: IBC transfer scenarios #10852

0xpatrickdev · 2025-01-16T00:08:25Z

closes: #XXXX
refs: #XXXX

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages · 2025-01-16T00:10:45Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`41768bf`
Status:	✅ Deploy successful!
Preview URL:	https://3f0753f9.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-ibc-xfer-test.agoric-sdk.pages.dev

View logs

mhofman

I think I'm confusing myself on which hook does what. Do we have a flow diagram somewhere?

mhofman · 2025-01-23T05:58:20Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+func (k Keeper) PacketStore(ctx sdk.Context, ourOrigin types.PacketOrigin, ourPort string, ourChannel string, sequence uint64) (storetypes.KVStore, []byte) {
+	key := fmt.Sprintf("%s/%s/%s/%v", ourOrigin, ourPort, ourChannel, sequence)
+	packetKey := []byte(key)
+	return prefix.NewStore(ctx.KVStore(k.key), []byte(packetDataStoreKeyPrefix)), packetKey
+}


Maybe make a reference to https://github.com/agoric-labs/ibc-go/blob/v6.3.1-alpha.agoric.2/modules/core/24-host/keys.go#L183-L187 but mention we need to support both directions (hence the extra origin)?

Btw I like the paths used by IBC better because it denotes what each path element is in the name

originalData/{src|dst}/ports/{ourPort}/channel/{ourChannel}/sequences/{sequence}

mhofman · 2025-01-23T06:24:18Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+	if StorePacketData && !bytes.Equal(strippedPacket.GetData(), packet.GetData()) {
+		packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)
+		if !packetStore.Has(packetKey) {
+			packetStore.Set(packetKey, packet.GetData())


Why are we remembering the packet data when receiving an ack? Shouldn't we assert we had saved packet data if we rewrote the packet, and delete instead?

Yes, your suggestion is correct.

mhofman · 2025-01-23T06:27:48Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+	origPacket := packet
+	packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)
+	if packetStore.Has(packetKey) {
+		origPacket.Data = packetStore.Get(packetKey)


I'm confused, why do we need to restore the original packet's data? Wouldn't a received timeout packet contain the original packet data in the first place?

Correct. I've updated this to be the same logic as InterceptOnAckPacket above.

mhofman · 2025-01-23T06:30:07Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+		packet.GetDestPort(), packet.GetDestChannel(),
+		timeout, packet.GetTimeoutTimestamp(),
+	)
+	packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)


If we're writing an ack, wouldn't we be the destination / receiver? Why are we using a src origin here?

Correct, it should be PacketDst.

michaelfig · 2025-01-27T18:39:42Z

I think I'm confusing myself on which hook does what. Do we have a flow diagram somewhere?

There is some descriptive text in https://github.com/Agoric/agoric-sdk/pull/10852/files#diff-9f5147516ace799597d78cf5abf1f9d93de4304489b31628b02bc471143c4b84L13 that may help.

mhofman

Still reviewing, especially the tests, but I'm still not sure I understand the order of hooks invocation, especially in the case of ack received for sent ibc packets.

mhofman · 2025-01-27T19:00:58Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+	if StorePacketData && !bytes.Equal(strippedPacket.GetData(), packet.GetData()) {
+		packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketSrc, packet)
+		if packetStore.Has(packetKey) {


Right now this handles the case where stripped packet != packet && store.has(packet).

We have 3 other cases:

store.has(packet) && stripped packet == packet. This arguably shouldn't happen, but I don't see a reason to not delete the stored data (if it matches the packet)?

stripped packet != packet && !store.has(packet). Could this happen? Wouldn't IBC guarantee that we only see an ack only once for packet we actually sent? Even in the case of timeout. If true, is it worth asserting this is an unexpected packet or just trust IBC that this will never happen?

stripped packet == packet && !store.has(packet). This is the normal case where the packet has not be modified and we have nothing to do.

mhofman · 2025-01-27T19:03:47Z

golang/cosmos/x/vtransfer/keeper/keeper.go

@@ -180,6 +297,18 @@ func (k Keeper) InterceptOnTimeoutPacket(
 	if err != nil {
 		return err
 	}
+


It seems this is specific to timeout packets received from a relayer. Is there ever a case where the local IBC logic generates a timeout when it receives an update client past the timeout (after verifying the remote chain didn't include the sent packet in its state)

mhofman · 2025-01-27T19:05:44Z

golang/cosmos/x/vtransfer/keeper/keeper.go

+	packetStore, packetKey := k.PacketStoreFromOrigin(ctx, types.PacketDst, packet)
+	if packetStore.Has(packetKey) {
+		origPacket.Data = packetStore.Get(packetKey)
+		packetStore.Delete(packetKey)
+	}


How does this relate to the deletion done in func (i4 *ics4Wrapper) WriteAcknowledgement ?

Is there a case where we may have an async ack that no longer has the original data saved?

mhofman · 2025-01-27T19:09:15Z

multichain-testing/test/account-balance-queries.test.ts

@@ -186,6 +186,6 @@ const queryAccountBalance = test.macro({
 test.serial(queryAccountBalances, 'osmosis');
 test.serial(queryAccountBalances, 'cosmoshub');
 test.serial(queryAccountBalances, 'agoric');
-test.serial(queryAccountBalance, 'osmosis');
+test.serial.failing(queryAccountBalance, 'osmosis');


Why are these failing now?

- min fees were set to 0 to allow the relayers to come up. likely not the best approach, but the most expedient for now

- allow callers to set broadcast mode and fees - allow callers to specify another binary, like osmosisd or gaiad

0xpatrickdev force-pushed the pc/ibc-xfer-test branch 2 times, most recently from 21dbe29 to 014071b Compare January 21, 2025 17:23

michaelfig force-pushed the pc/ibc-xfer-test branch from 014071b to 33915c1 Compare January 22, 2025 16:15

michaelfig changed the title ~~test: ibc transfer scenarios~~ test: IBC transfer scenarios Jan 23, 2025

michaelfig self-assigned this Jan 23, 2025

michaelfig added the force:integration Force integration tests to run on PR label Jan 23, 2025

mhofman reviewed Jan 23, 2025

View reviewed changes

mhofman self-requested a review January 23, 2025 06:44

michaelfig force-pushed the pc/ibc-xfer-test branch from 3a4bb6b to 560a63a Compare January 25, 2025 02:27

michaelfig marked this pull request as ready for review January 27, 2025 05:27

michaelfig requested a review from a team as a code owner January 27, 2025 05:27

michaelfig requested review from mhofman and removed request for mhofman January 27, 2025 16:12

mhofman reviewed Jan 27, 2025

View reviewed changes

mhofman requested review from michaelfig and mhofman January 27, 2025 19:15

michaelfig and others added 12 commits January 28, 2025 13:54

build(deps): regenerate some yarn.locks

ac4bbd0

fix(vat-bank): allow numeric or string nonces

f860604

fix(telemetry): initialize empty flight-recorders

706b51b

fix(vtransfer): some tweaks for ack handling

8dbd0ed

fix(address-hooks): reuse packet bytes if no properties changed

07d73c3

chore(vibc): make RawAcknowledgement public

cbe6c44

chore: bump to latest versions

f20850f

- min fees were set to 0 to allow the relayers to come up. likely not the best approach, but the most expedient for now

chore: extend makeAgd with more opts

7014726

- allow callers to set broadcast mode and fees - allow callers to specify another binary, like osmosisd or gaiad

test: pfm transfer

8892197

chore: track queries

16751ee

chore(multichain-testing): add make rebuild

18eeaf1

chore(multichain-testing): fix the lint

34f9418

michaelfig added 3 commits January 28, 2025 14:00

test(multichain-testing): skip or mark new failed tests

aa6292f

test(multichain-testing): solve some failures

cf41d2c

test(fast-usdc): make txHash collisions less likely

41768bf

michaelfig force-pushed the pc/ibc-xfer-test branch from cfa9c95 to 41768bf Compare January 28, 2025 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: IBC transfer scenarios #10852

test: IBC transfer scenarios #10852

0xpatrickdev commented Jan 16, 2025

cloudflare-workers-and-pages bot commented Jan 16, 2025 •

edited

Loading

mhofman left a comment

mhofman Jan 23, 2025

michaelfig Jan 26, 2025

mhofman Jan 23, 2025

michaelfig Jan 26, 2025

mhofman Jan 23, 2025

michaelfig Jan 26, 2025

mhofman Jan 23, 2025

michaelfig Jan 26, 2025

michaelfig commented Jan 27, 2025

mhofman left a comment

mhofman Jan 27, 2025

mhofman Jan 27, 2025

mhofman Jan 27, 2025

mhofman Jan 27, 2025

test: IBC transfer scenarios #10852

Are you sure you want to change the base?

test: IBC transfer scenarios #10852

Conversation

0xpatrickdev commented Jan 16, 2025

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jan 16, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Jan 27, 2025

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jan 16, 2025 •

edited

Loading